feat(model-apps): verify what persona security roles actually grant - #423
feat(model-apps): verify what persona security roles actually grant#423Akshay Maloo (akshaymaloo) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a proposal-only design note for “JTBD probes” to extend the /app-builder workflow beyond the current handoff point (“Then open the app in the browser”) by making personas[].jobs[].surfaces[] actionable. It also updates the app-builder roadmap to reference the proposal and position it in the “Quality & docs” phase.
Changes:
- Add
jtbd-probe-design.mddescribing a four-rung approach: offline surface resolution → offline deep-link routing → Playwright drive → Dataverse-backed assertions. - Update
app-builder-roadmap.mdto include the JTBD probes proposal and link to the new design note.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| plugins/model-apps/docs/jtbd-probe-design.md | New design note proposing an incremental, runged approach to executable JTBD probes, including constraints and anti-goals. |
| plugins/model-apps/docs/app-builder-roadmap.md | Adds a roadmap item referencing the JTBD probes proposal and links to the new design note. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
plugins/model-apps/docs/jtbd-probe-design.md:133
- Same as above:
lib/surface-route.jssuggests a directory that doesn’t exist in this plugin. Consider aligning the proposed module location with the establishedscripts/lib/convention.
`lib/surface-route.js`: a resolved ref → an MDA deep link.
plugins/model-apps/docs/jtbd-probe-design.md:111
- The proposed module path
lib/surface-resolver.jsdoesn’t match the existing repo layout (there is noplugins/model-apps/lib/; shared code lives underscripts/lib/). Using the correct path in the design note will reduce implementation ambiguity.
This issue also appears on line 133 of the same file.
A new pure module `lib/surface-resolver.js`:
plugins/model-apps/docs/jtbd-probe-design.md:104
- Section numbering is inconsistent: this is labeled “## 3.” after “## 3a.” and “## 3b.” above, which makes references harder to follow in reviews and future discussions. Consider renaming this heading to keep the sequence monotonic.
## 3. Four rungs, each shippable alone
plugins/model-apps/docs/jtbd-probe-design.md:215
- This sentence says to prototype auth before writing any of Rungs 0–3, but earlier the doc explicitly stages Rungs 0–1 to be offline and valuable even if auth never works unattended. Tightening this to “Rungs 2–3” keeps the sequencing internally consistent.
for. **Prototype this before writing any of Rungs 0–3.** If a persisted profile cannot be made to
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
plugins/model-apps/scripts/verify-model-app.js:113
privilegedepthmaskis documented as a bitmask; mapping only the exact values 1/2/4/8 will return an empty depth string if the service ever returns a combined mask (e.g. 3/5/6/7), causing false failures in the role-privileges subset comparison. Translate masks by taking the most-permissive bit set (Global > Deep > Local > Basic).
const DEPTH_BY_MASK = { 1: 'Basic', 2: 'Local', 4: 'Deep', 8: 'Global' };
const rows = await sdk.queryRecords('roleprivileges', {
select: ['privilegeid', 'privilegedepthmask'],
filter: `roleid eq ${roleId}`,
top: 5000,
plugins/model-apps/docs/jtbd-probe-design.md:3
- The doc still states "proposal, not yet built" / "Nothing here is implemented", but this PR actually implements rung 0 (surface resolution + lint/verify wiring) and the role privilege-depth verification. Update the status/title to reflect that parts have shipped and clarify which rungs remain proposed, otherwise readers (and the PR description) will be inaccurate.
# JTBD probes — design note (proposal, not yet built)
**Status:** proposal for review. Nothing here is implemented.
| entityPrivileges: async (logical) => { | ||
| const name = String(logical).toLowerCase(); | ||
| const url = `/EntityDefinitions(LogicalName='${odataLit(name)}')?$select=LogicalName,Privileges`; | ||
| const res = await opts.httpClient.get(url); | ||
| if (!res || res.status < 200 || res.status >= 300) return null; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (4)
plugins/model-apps/scripts/verify-model-app.js:127
- readerFor() now always exposes entityPrivileges()/rolePrivileges(), but entityPrivileges depends on opts.httpClient. build-model-app.js calls readerFor(provisionSdk, …) without passing httpClient, so role-privilege verification will fail closed for every persona (entityPrivileges throws, entityPrivileges map stays empty, compareRolePrivileges reports every declared privilege as unreadable). Either thread httpClient through build-model-app’s verify reader, or gate these readers so they are only present when an httpClient is supplied.
entityPrivileges: async (logical) => {
const name = String(logical).toLowerCase();
const url = `/EntityDefinitions(LogicalName='${odataLit(name)}')?$select=LogicalName,Privileges`;
const res = await opts.httpClient.get(url);
if (!res || res.status < 200 || res.status >= 300) return null;
plugins/model-apps/scripts/lib/surface-resolver.js:67
- declaredSurfaces() has the same malformed-shape hazard:
for (const p of spec.personas || [])will throw if personas is a truthy non-array (object/string), and(p && p.jobs) || []will throw if jobs is truthy non-array. If this helper is meant to be safe on WIP specs, use Array.isArray guards for personas/jobs/surfaces.
function declaredSurfaces(spec) {
const out = [];
for (const p of spec.personas || []) {
for (const j of (p && p.jobs) || []) {
for (const s of (j && j.surfaces) || []) {
if (typeof s === 'string' && s.trim()) out.push({ persona: (p.persona || '').trim(), job: (j.name || '').trim(), surface: s.trim() });
plugins/model-apps/scripts/verify-model-app.js:113
- privilegedepthmask is a bitmask; Dataverse can return combined values (e.g., 7 or 15) representing the max depth. Mapping only {1,2,4,8} turns combined masks into an empty depth string, which then reads as “held at , below declared …” and can false-fail privilege checks. Decode the bitmask by taking the highest set bit (Global > Deep > Local > Basic).
const DEPTH_BY_MASK = { 1: 'Basic', 2: 'Local', 4: 'Deep', 8: 'Global' };
const rows = await sdk.queryRecords('roleprivileges', {
select: ['privilegeid', 'privilegedepthmask'],
filter: `roleid eq ${roleId}`,
top: 5000,
plugins/model-apps/scripts/lib/surface-resolver.js:55
- buildIndex() iterates with
for (const x of spec.<field> || []). If a malformed/in-progress spec has a truthy non-array (e.g.,{ views: {} }),spec.views || []yields an object andfor..ofthrows, contradicting the module’s “tolerates a malformed spec” intent. Guard each collection with Array.isArray (similar to spec-lint’s arrOf()) before iterating.
This issue also appears on line 62 of the same file.
for (const v of spec.views || []) add(v && v.name, { kind: 'view', name: v.name, entity: v.entity });
for (const f of spec.forms || []) add(f && f.name, { kind: 'form', name: f.name, entity: f.entity });
for (const d of spec.dashboards || []) add(d && d.name, { kind: 'dashboard', name: d.name });
for (const p of spec.pages || []) {
// A page is referenced by stable KEY in schemaVersion 2 and by display name in prose, so accept
Two metadata-only verification gaps, both found while auditing what `verify`
can and cannot prove. Neither needs a live browser or new infrastructure.
1. verify now proves what a persona role GRANTS, not just that it exists.
The `role` check asserted only that a role ROW exists carrying the SDK
ownership marker. It never looked at privileges - so a role created with the
wrong access, or one whose privilege write failed after the row landed,
verified clean.
The new `role-privileges` check resolves each declared (entity, access) to
its Dataverse PrivilegeId from the SAME metadata source the SDK writes
against - EntityDefinitions(...)?$select=Privileges - and asserts the role
holds it at AT LEAST the declared depth.
SUBSET, not equality, and lib/role-privileges.js records why: appAccess
injects appmodule read, unioned jobs escalate a shared entity+access to the
max declared scope, and distinct entities can share ONE Dataverse privilege
(a role holds one depth per privilege). Equality would fail on all three
while telling us nothing true. Fails CLOSED on an unreadable role or table.
The read deliberately does NOT go through sdk.fetchEntityMetadata: that
returns a projected shape which drops Privileges entirely, so routing through
it would have silently reported every privilege as unreadable.
2. personas[].jobs[].surfaces[] is checked instead of documentary.
app-spec.js validated each entry as a non-empty string and stopped;
spec-lint warned only when the array was EMPTY. So a job could name "My Open
Work Orders" when no such view existed anywhere in the spec and every gate
passed.
lib/surface-resolver.js resolves each entry against the spec's own views,
forms, pages (key OR name), dashboards, tables and sitemap titles. spec-lint
WARNS on no match - a warning, never an error, because app-spec.js is loose
on purpose: a surface may legitimately name an out-of-the-box artifact this
spec does not author.
verify adds a `job-surface` rollup - a PURE rollup over checks already
computed, so it costs no extra reads - reporting a deployed failure as the
job it broke ("persona P can no longer do job J") rather than only "view X
is missing".
Both wire into verifySpec's existing READER-GATED seam (the pattern
entityRelationships / commandBar already use), so an existence-only reader
behaves exactly as it did before.
Tests: 28 new (1446 -> 1474 pass, 0 fail). Both features red-green verified:
disabling the lint wiring fails 2, disabling the privilege check fails 3.
Evals 159 pass, 6/6 validators.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 42626da2-b66f-4162-acaa-b1127ef23d89
1c0d8c8 to
e850df8
Compare
|
Superseded by #425 — same commit, on a branch renamed to match the shipped scope. The exploratory design note that was originally part of this PR has been dropped; only the two verification improvements ship. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
plugins/model-apps/scripts/verify-model-app.js:222
entityPrivilegesnow relies onopts.envUrlto build an absolute Dataverse Web API URL. The currentreaderFor(...)call does not pass the env URL, so the privilege metadata read will return null and the newrole-privilegescheck will fail-closed. Pass the env URL through the reader options (and ensure other callers of the exportedreaderFordo the same).
const r = await verifySpec(spec, readerFor(sdk, appUniqueName(spec), { genpageCli, workspaceDir, httpClient }));
plugins/model-apps/scripts/verify-model-app.js:128
entityPrivilegescallsopts.httpClient.get()with a relative URL (/EntityDefinitions...). The injectedcreateAzHttpClientexplicitly refuses non-absolute URLs (it expects the SDK-style full URLhttps://<org>/api/data/v9.x/...), so this will throw and make therole-privilegescheck fail-closed even in healthy environments.
const name = String(logical).toLowerCase();
const url = `/EntityDefinitions(LogicalName='${odataLit(name)}')?$select=LogicalName,Privileges`;
const res = await opts.httpClient.get(url);
if (!res || res.status < 200 || res.status >= 300) return null;
return (res.body && res.body.Privileges) || null;
plugins/model-apps/scripts/lib/role-privileges.js:8
- Comment typo: this line starts with
////, which looks accidental and makes the header harder to read.
//// SUBSET, not equality. We assert the role holds AT LEAST every declared privilege at AT LEAST the
Two verification gaps in
/app-builder, both metadata-only — no new infrastructure, no live-browser dependency.1.
verifynow proves what a persona security role GRANTSThe
rolecheck asserted only that a role row exists carrying the SDK ownership marker. It never looked at privileges — so a role created with the wrong access, or one whose privilege write failed after the row landed, verified clean.The new
role-privilegescheck resolves each declared(entity, access)to its DataversePrivilegeIdfrom the same metadata source the SDK writes against, and asserts the role holds it at at least the declared depth.Subset, not equality —
lib/role-privileges.jsrecords why. Equality would false-fail on three legitimate causes:appAccessinjectsappmodulereadFails closed on an unreadable role or table.
2.
personas[].jobs[].surfaces[]is checked instead of documentaryapp-spec.jsvalidated each entry as a non-empty string and stopped;spec-lintwarned only when the array was empty. So a job could name"My Open Work Orders"when no such view existed anywhere in the spec — and every gate passed.lib/surface-resolver.jsnow resolves each entry against the spec's own views, forms, pages (key or name), dashboards, tables and sitemap titles.spec-lintwarns on no match — a warning, never an error, becauseapp-spec.jsis loose on purpose: a surface may legitimately name an out-of-the-box artifact this spec does not author.verifyadds ajob-surfacerollup — a pure rollup over checks already computed, so no extra reads — reporting a deployed failure as the job it broke ("persona P can no longer do job J") rather than only "view X is missing".Compatibility
Both wire into
verifySpec's existing reader-gated seam — the patternentityRelationships/commandBaralready use — so an existence-only reader behaves exactly as it did before.Verification